Skip to content

Avoid storing last_changed in the database if its the same as last_updated#71843

Merged
balloob merged 4 commits intohome-assistant:devfrom
bdraco:no_store_last_changed_if_same
May 14, 2022
Merged

Avoid storing last_changed in the database if its the same as last_updated#71843
balloob merged 4 commits intohome-assistant:devfrom
bdraco:no_store_last_changed_if_same

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented May 14, 2022 •
edited
Loading

Proposed change

Data PR home-assistant/data.home-assistant#214

Since last_updated and last_changed are stored as strings in the database they take up
quite a bit of space.

In testing various production instances, ~75%-96% of last_changed was the same as last_updated

sqlite> select count(*) from states where last_updated == last_changed;
1353902
sqlite> select count(*) from states where last_updated != last_changed;
44720

sqlite> select count(*) from states where last_updated == last_changed;
1308420
sqlite> select count(*) from states where last_updated != last_changed;
351063

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

probot-home-assistant bot commented May 14, 2022

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (logbook) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@probot-home-assistant
Copy link
Copy Markdown

probot-home-assistant bot commented May 14, 2022

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (recorder) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 14, 2022

Need to write docs for this.

bdraco added 2 commits May 13, 2022 23:28
@bdraco
fix test flag (will not change results)
973ef32
@bdraco
allow minimal_response + significant_changes_only=False since it does…
aa5cc9d 
… actually work because of the `split_entity_id(ent_id)[0] in NEED_ATTRIBUTE_DOMAINS` guard
@bdraco bdraco marked this pull request as ready for review May 14, 2022 18:28
@bdraco bdraco requested a review from a team as a code owner May 14, 2022 18:28
@bdraco bdraco mentioned this pull request May 14, 2022
Merged
balloob
balloob approved these changes May 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I think that we can also leverage this on the frontend and not send both over if they are the same.

@balloob balloob merged commit 8c2743b into home-assistant:dev May 14, 2022
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 15, 2022

Nice! I think that we can also leverage this on the frontend and not send both over if they are the same.

Thats a good idea. I think we can cleanup https://github.com/home-assistant/frontend/blob/dev/src/data/history.ts#L260 a bit more once home-assistant/frontend#12662 merges but I don't want to make it any bigger

@bdraco bdraco mentioned this pull request May 15, 2022
Merged
9 tasks
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 15, 2022

Nice! I think that we can also leverage this on the frontend and not send both over if they are the same.

#71886

@bdraco bdraco mentioned this pull request May 15, 2022
Merged
22 tasks
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@balloob balloob balloob approved these changes

Assignees

No one assigned

Labels

cla-signed code-quality core has-tests integration: logbook integration: recorder

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bdraco @balloob @homeassistant